-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: Propagate Namespace correctly to Helm #5940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Propagate Namespace correctly to Helm #5940
Conversation
Welcome @Skaronator! |
Hi @Skaronator. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@koba1t Could you please take a look on this and do |
hi @ncapps @varshaprasad96 @koba1t this PR addresses a bugfix for #5566 , which is a highly upvoted issue at the moment. It’s been open for a while, so could we please review and prioritize it? Thanks! |
/ok-to-test |
/assign |
Thanks for your help that difficult issue! So, I want to verify whether this feature works when a kustomization.yaml containing helmCharts is referenced via resources from another kustomizationFile. Please add test cases for this purpose. Example
And, Please also add tests for cases where the Example
Additionally, having tests for cases where the same namespace is configured for both namespace transformer and helmCharts.namespace would help ensure reliable regression prevention. Example
|
This PR has multiple commits, and the default merge method is: merge. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@koba1t Thanks for the review and suggestion for the additional test case. During implementation, I discovered an underlying issue where Kustomize doesn't properly propagate the I've fixed this by ensuring that the parent kustomization's namespace is always propagated to child kustomizations when they don't already have a namespace defined. This ensures Helm receives the correct namespace during chart I've added comprehensive test cases for this specific use case, and the existing |
Hi @Skaronator /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: koba1t, Skaronator The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks! |
For awareness: We're seeing substantial performance degradation in some of our overnight runs against
This test runs what's described in #2869. We have a significantly more complex test that has been running in around 65 to 70 seconds; as of today that test hits our run timeout of an hour. I hope to have more time this weekend to provide more details. Edited to add: Yes, the performance change is due to this commit.
Regarding the faster run times here, the environment for this run was different than the one further above. |
I profiled the test run. CPU comparisons:
Memory comparisons:
|
Hmm.... @ephesused |
I'll take a look if we can improve the performance. My guess is that the Namespace Transformer get applied multiple times in a nested way which is not necessary. |
Summary
This PR introduces a bugfix for issue #5566, which is currently the 6th most upvoted issue in the Kustomize repository.
Bugfix Details
The fix modifies how
namespace
values are handled withinkustomization.yaml
when usinghelmCharts
:namespace
is set at the top level of thekustomization.yaml
, it will always be propagated to the Helm CLI command.helmCharts
entry specifies its ownnamespace
, that value takes precedence and overrides the top-level one.NamespaceTransformer Behavior
Additionally, the
NamespaceTransformer
will now skip all manifests generated by Helm charts. This aligns with the principle that Helm charts should manage their own namespaces internally.This change also enables charts to deploy resources across multiple namespaces—an essential capability for many popular charts such as the
cert-manager
Helm chart, which installs RBAC resources into thekube-system
namespace.